Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 226 - Don't create a new Faraday instance on every request #233

Merged
merged 4 commits into from
Feb 3, 2012

Conversation

oliverbarnes
Copy link
Contributor

Refactored following the discussion on the issue

sferik added a commit that referenced this pull request Feb 3, 2012
Issue 226 - Don't create a new Faraday instance on every request
@sferik sferik merged commit e3b8442 into sferik:master Feb 3, 2012
@sferik
Copy link
Owner

sferik commented Feb 3, 2012

Thanks @oliverbarnes! Also, thanks for the code reviews @laserlemon and @mislav.

@oliverbarnes
Copy link
Contributor Author

Hey guys, sorry for not responding sooner, I see this has already been implemented and merged.

I don't follow what's bad in memoizing the connection, though. from our exchange with @mislav, I understood it didn't matter:

It makes sense to pass connection options to connection even if it's going to be memoized, if these options aren't expected to change (such as SSL settings, endpoint, etc.).

@laserlemon
Copy link
Collaborator

The endpoint option actually is a request-specific option. Calling the search method hits an entirely different endpoint than fetching your user.

@mislav
Copy link
Contributor

mislav commented Jun 4, 2012

@laserlemon: One Faraday Connection per each endpoint is kept.

This isn't needed, however. If I were designing this, I would still have just one Connection object to the main endpoint, and change the hostname of the endpoint per each request that's not to the main one.

@laserlemon
Copy link
Collaborator

@mislav Maybe I'm misunderstanding you, but your link demonstrating that one connection is kept per endpoint is because of my very recent pull request.

Also, I agree with your design idea. I'd love it if the connection was just a one-off method and the Faraday middleware could switch the endpoint/hostname based on the incoming request. Unfortunately, how to do that isn't immediately coming to mind. But the endpoint option is the only real option remaining so it feels like we're very close!

@mislav
Copy link
Contributor

mislav commented Jun 4, 2012

@laserlemon: I didn't know you were responsible for it. I just looked at the master version, and assumed it was like that for longer.

Here, I implemented your wish. It wasn't that hard.

@laserlemon
Copy link
Collaborator

Cool! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants